Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: migrate TesterApp to react-native-test-app #629

Merged
merged 24 commits into from
Jun 11, 2024

Conversation

szymonrybczak
Copy link
Contributor

@szymonrybczak szymonrybczak commented May 26, 2024

TODO:

  • Make sure that Release mode works
  • Specify rest options through app.json e.g. New Architecture, build modes, schemas.

Summary

In this Pull Request I've migrated TesterApp to react-native-test-app.

CleanShot 2024-05-26 at 20 05 38

Test plan

App should work in the same way as before.

@szymonrybczak szymonrybczak requested a review from jbroma May 26, 2024 18:06
Copy link

vercel bot commented May 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
repack-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 11, 2024 2:48pm

Copy link

changeset-bot bot commented May 26, 2024

⚠️ No Changeset found

Latest commit: 400213d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Comment on lines 27 to 30
# Automatically convert third-party libraries to use AndroidX
android.enableJetifier=true
# Jetifier randomly fails on these libraries
android.jetifier.ignorelist=hermes-android
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK jetifier is no longer necessary for RN libraries as all moved on to AndroidX, isn't it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is inside of TesterApp, if we can disable jetifier then lets do it 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if all libs are migrated what's blocking us from removing it from the template? :) probably backward compatability, i have no idea how long we should keep it 🤔

https://github.com/react-native-community/template/blob/cd5e21b702ef5c08c74796d19dd72bdb05775d17/template/android/gradle.properties#L24-L25

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packages/TesterApp/.watchmanconfig Outdated Show resolved Hide resolved
Comment on lines 27 to 30
# Automatically convert third-party libraries to use AndroidX
android.enableJetifier=true
# Jetifier randomly fails on these libraries
android.jetifier.ignorelist=hermes-android
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is inside of TesterApp, if we can disable jetifier then lets do it 🚀

packages/TesterApp/app.json Outdated Show resolved Hide resolved
@jbroma
Copy link
Member

jbroma commented Jun 3, 2024

Needed to add a bootstrap HOC as a workaround for development - apparently anything done before registering the main component causes native modules initialization to fail for some reason?

@tido64
Copy link
Contributor

tido64 commented Jun 4, 2024

Needed to add a bootstrap HOC as a workaround for development - apparently anything done before registering the main component causes native modules initialization to fail for some reason?

@jbroma: Do you have repro steps? I reverted badea9d but it seems to work fine?

Android iOS
Screenshot_1717488364 image

@jbroma
Copy link
Member

jbroma commented Jun 4, 2024

@tido64 hey, thanks for looking into this 🙏

did you restart the dev server after reverting the latest commit? This issue seems to only happen on the first load when the dev server needs to bundle everything from scratch. Here's some screenshots from the app without the latest commit:

During bundling Finished bundling
Screenshot 2024-06-04 at 10 10 32 Screenshot 2024-06-04 at 10 10 41

@jbroma
Copy link
Member

jbroma commented Jun 4, 2024

@tido64 I've narrowed it down to a more specific scenario, here are steps to repro:

cd packages/TesterApp
# start a fresh instance of dev-server
pnpm start
# in a different pane/window
pnpm android

First load of the newly installed app should cause the issue as shown above upon finish bundling (the one with AsyncStorage being null)

I've also confirmed that iOS is unaffected, this is only on Android

@szymonrybczak szymonrybczak marked this pull request as ready for review June 6, 2024 09:54
@jbroma
Copy link
Member

jbroma commented Jun 10, 2024

Both release modes are working now:

  • for iOS we first need to run pnpm bundle:ios and then do bundle exec pod install (inside ios dir) so that the release bundles get copied to the app properly. Important thing to note here is that after running pod install, the target is set to Debug, so it might needed to be changes to Release manually.
  • for Android: for assets to be properly picked up it's best that we clean first (or rebuild) and then run the app in release mode

The commands are quite long in package.json, we might want to pull some of that stuff out into separate files in scripts dir. Ideally all issues above could be included in release scripts so that we don't need to remember about that stuff.

cc @szymonrybczak

@szymonrybczak
Copy link
Contributor Author

Added script:
pnpm run ios:release - should bundle and app in Release mode
pnpm run android:release - should bundle and app with ./assembleRelease

Copy link
Member

@jbroma jbroma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GJ! small nits, except for that I think we finally got this ready to be merged 💪

packages/TesterApp/scripts/release.js Show resolved Hide resolved
packages/TesterApp/scripts/release.js Outdated Show resolved Hide resolved
packages/TesterApp/scripts/release.js Outdated Show resolved Hide resolved
Copy link
Member

@jbroma jbroma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@jbroma jbroma merged commit 7fc5a6b into main Jun 11, 2024
5 checks passed
@jbroma jbroma deleted the feat/migrate-testerapp-to-rnta branch June 11, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants